Test with Julienne #169

rouson · 2024-12-28T19:03:40Z

This PR replaces PR #133 and accomplishes the following:

Switch from testing with Veggies to testing with Julienne.
Fix issue Unit tests for prif_stop and prif_error_stop make fragile non-portable assumptions #137 by moving the termination tests to a separate fpm project in the new test-termination/ subdirectory as suggested in comment r1761996357.
Define a new prif_test_t type that extends Julienne's test_t type and override test_t's report type-bound procedure, replacing parallel Fortran features with PRIF calls.

The primary motivation for the switch to Julienne is complexity reduction: the Julienne software stack contains 3850 lines of code versus 30513 in Veggies as determined by executing following command on the current main branch of both repositories:

wc -l `find src example test build/dependencies -iname '*.f90'

As a consequence, the test suite runs much faster even after accounting for some possible reduction in test coverage because this test suite was originally developed in 3-4 months ago so any tests added to the Veggies-based test suite since then are not represented here. Determining whether and to what extent the test coverage is actually diminished is on the To Do list below.

To Do

Edit the CI scripts to run the termination tests in the new test-termination subdirectory.
Review and edit the test/ and test-termination/ subdirectories to match the test coverage of the previous Veggies-based tests.

Test coverage: * prif_init * prif_allocate, prif_deallocate, prif_allocate_coarray, prif_deallocate_coarray * prif_co_broadcast, prif_co_max, prif_co_min, prif_co_sum, prif_co_reduce * prif_image_index, prif_num_images, prif_this_image_no_coarray * prif_put, prif_get * prif_sync_all * prif_team_type, prif_form_team, prif_change_team, prif_end_team

This commit replaces veggies and iso_varying_string in the fpm manifest with julienne and assert.

git mv main.{f,F}90

This commit fixes a problem that caused seg faults when running subsets of tests. Previously, if the subset didn't include the prif_init test, then prif_init was never called. This commit calls prif_init in test/main.F90 before running the tests. Consequently, the test for normal execution of prif_init now happens via an assertion rather than in a unit test.

As suggested in the following comment, this commit defines a separate project for testing program termination: #133 (comment)

This commit removes code that was temporarily inserted when diagnosing a flang bug and uncomments the correct code.

This commit defines a new prif_test_t type that extends Julienne's test_t type in ordero to override test_t's "report" type-bound procedure in order to replace parallel Fortran featuers with PRIF calls.

This eliminates a do loop in prif_test_t's "report" tybe-bound procedure.

everythingfunctional

I haven't reviewed the changes the tests yet, but I have a few higher level comments so far.

You need to add an fpm.toml file in the test-termination folder. At present the instructions in the test-termination/README.md do not work. The fpm.toml I created is listed below.
It is unclear from the output whether the tests pass or fail. I would recommend putting together some sort of script that checks the output of each individual test rather than have users try to inspect the outputs and determine pass/fail manually. This should make getting the CI to work easier as well.
I would have made the switch to Julienne a separate PR, as the stop tests don't really depend on the test framework and vice-versa.
I think on balance the switch to Julienne might be the right move, but I think it's worth pointing out some of the important differences:
1. You get less detailed output from running the test suite. i.e. it doesn't report how many or what values are being compared. This could make debugging failing tests (or erroneously passing tests) more difficult.
2. The framework is simpler, and it is one you control.
I think you could adapt veggies in a similar way as you did with julienne, so if you're curious to try it, I could point you at the few things you'd want/need to overload.

example fpm.toml file

name = "test-termination"
version = "0.4.1"
license = "BSD"
author = ["Damian Rouson", "Brad Richardson", "Katherine Rasmussen", "Dan Bonachea"]
maintainer = "[email protected]"
copyright = "2021-2024 UC Regents"

[dependencies]
caffeine = {path = ".."}

[build]
link = ["gasnet-smp-seq", "gcc", "m"]

The new text at the bottom of the test-termination/README.md specifies the environment variable definitions used in running the termination tests.

rouson · 2025-01-02T20:53:14Z

@everythingfunctional thanks for the quick review!

You need to add an fpm.toml file in the test-termination folder. At present the instructions in the test-termination/README.md do not work. The fpm.toml I created is listed below.

Good catch. I just pushed my fpm.toml, which I mistakenly didn't add before. This required git add -f because fpm.toml is in the .gitignore due to the top-level manifest being generated by install.sh.

It is unclear from the output whether the tests pass or fail. I would recommend putting together some sort of script that checks the output of each individual test rather than have users try to inspect the outputs and determine pass/fail manually. This should make getting the CI to work easier as well.

That's the plan. I'm hoping @ktras can work on this.

I would have made the switch to Julienne a separate PR, as the stop tests don't really depend on the test framework and vice-versa.

I agree. If we decide against the switch to Julienne, I'll create a separate PR to replace the old program termination tests with the new ones.

I think on balance the switch to Julienne might be the right move, but I think it's worth pointing out some of the important differences:

You get less detailed output from running the test suite. i.e. it doesn't report how many or what values are being compared. This could make debugging failing tests (or erroneously passing tests) more difficult.

This morning's release of Julienne 1.6.0 adds this information in diagnostic output. This issue has been bugging me for a while, but it took some focused time over the break to think of how to add such diagnostic output in backwards-compatible way. Of course, there aren't many projects using Julienne yet (two-ish) but it would be nice to not have to rewrite multiple projects' test suites all at once. Fortunately I finally figured out a backwards-compatible path and that's in Julienne 1.6.0.

The framework is simpler, and it is one you control.

I think you could adapt veggies in a similar way as you did with julienne, so if you're curious to try it, I could point you at the few things you'd want/need to overload.

Very cool. It would probably be useful info for learning purposes either way so pointers are welcome.

bonachea

Made some initial observations based on a quick skim of the high-level changes.

I have not yet dived into review of the test code.

bonachea · 2025-01-02T20:47:13Z

test/prif_co_reduce_test_m.F90

+    pure function alphabetize(lhs, rhs) result(first_alphabetically)
+      character(len=*), intent(in) :: lhs, rhs
+      character(len=len(lhs)) first_alphabetically
+      call_assert(len(lhs) == len(rhs))


Conditional assertions should generally not be used in correctness tests (unless the condition being checked is expected to impose a very significant execution penalty), because then compiling in production mode (where ASSERTIONS are off by default) will skip the assertion and undercut the coverage of the test.

Suggested change

call_assert(len(lhs) == len(rhs))

call assert_always(len(lhs) == len(rhs), "length parameter mismatch in alphabetize")

bonachea · 2025-01-02T20:48:16Z

test/main.F90

+
+  call stop_and_print_usage_info_if_help_requested
+  call prif_init(stat=init_status)
+  call_assert(init_status==successful)


Suggested change

call_assert(init_status==successful)

call assert_always(init_status==successful, "prif_init failed")

bonachea · 2025-01-02T20:57:30Z

manifest/fpm.toml.template

-veggies = {git = "https://gitlab.com/everythingfunctional/veggies", tag = "v1.1.3"}
-iso_varying_string = {git = "https://gitlab.com/everythingfunctional/iso_varying_string.git", tag = "v3.0.4"}
+julienne = {git = "https://github.com/berkeleylab/julienne.git", tag = "1.5.3"}
+assert = {git = "https://github.com/berkeleylab/assert.git", tag = "2.0.0"}


If we are adding a project dependence on berkeleylab/assert (which I DO support), then we should also excise the current caffeine_assert_s module which was added as a temporary workaround for a lack of a real assertion library.

However this change is also orthogonal to the Julienne transition and feels like it belongs in a separate PR.

bonachea · 2025-01-02T20:59:57Z

test-termination/fpm.toml

I'm not yet convinced that the proposed change fully resolves all six of the problems outlined in issue #137 that this PR claims to solve. However it might be a step in the right direction?

This commit appends `_c_bool` to the `quiet` argument of `prif_co_reduce` and removes the `pure` attribue from the encompassing procedure.

rouson mentioned this pull request Dec 28, 2024

Unit test with Julienne #133

Closed

16 tasks

rouson force-pushed the test-with-julienne branch from 189c949 to 886233a Compare December 28, 2024 19:46

rouson added 8 commits December 28, 2024 23:40

chore(fpm.toml): switch dev-dependencies

7ebab05

This commit replaces veggies and iso_varying_string in the fpm manifest with julienne and assert.

chore(test): preprocess test main

1d87e74

git mv main.{f,F}90

test(stopis): def new project to test termination

b0354e1

As suggested in the following comment, this commit defines a separate project for testing program termination: #133 (comment)

doc(test-termination): update README.md

b425d2f

doc(test-termination): update README.md

ad8f546

rm(co_reduce_test): rm temporary code

435d148

This commit removes code that was temporarily inserted when diagnosing a flang bug and uncomments the correct code.

rouson force-pushed the test-with-julienne branch from 886233a to 435d148 Compare December 29, 2024 04:51

rouson added 4 commits December 29, 2024 01:07

test: extend test_t, override report binding

ce8aa06

This commit defines a new prif_test_t type that extends Julienne's test_t type in ordero to override test_t's "report" type-bound procedure in order to replace parallel Fortran featuers with PRIF calls.

fix(co_min_test): simplify logic

c2b765f

fix(com_max_test): borrow logic from co_min_test

3f26d78

fix(co_reduce_test): improve logic

dec2d23

rouson marked this pull request as draft December 29, 2024 07:49

rouson marked this pull request as ready for review December 29, 2024 07:53

rouson requested review from everythingfunctional, bonachea and ktras December 29, 2024 08:04

rouson added 2 commits December 29, 2024 22:42

refac(prif_test_s): make co_all impure elemental

7268613

This eliminates a do loop in prif_test_t's "report" tybe-bound procedure.

chore(prif_test_s): rm redundant logic in report

2ffdbb6

everythingfunctional reviewed Jan 2, 2025

View reviewed changes

rouson added 2 commits January 2, 2025 12:38

doc(test-termination/README): specify environment

7ce33b7

The new text at the bottom of the test-termination/README.md specifies the environment variable definitions used in running the termination tests.

build(test-terminatio): add fpm.toml file

3b5c974

bonachea requested changes Jan 2, 2025

View reviewed changes

rouson added 3 commits January 2, 2025 14:30

build: rm external assert lib dependence

dd3fb3a

chore: rm unused parameter declarations

00ae2b3

test(co_reduce): fix standard violations

6d03002

This commit appends `_c_bool` to the `quiet` argument of `prif_co_reduce` and removes the `pure` attribue from the encompassing procedure.

rouson added 8 commits January 2, 2025 22:12

test(this_image): report diagnostics if test fails

9ae13e9

test(num_images): report diagnostics if test fails

1da811b

test(init): report diagnostics if test fails

9ab0a7f

test(image_index): report diagnostics for failures

5cc1538

test(co_broadcast):report diagnostics for failures

9341364

test(co_sum): report diagnostics for test failures

a529490

test(co_min): report diagnostics for test failures

277c746

test(co_max): report diagnostics for test failures

b3e90e5

bonachea mentioned this pull request Jan 4, 2025

Begin illustrating how to splice into veggies to work with prif #170

Draft

rouson added 2 commits January 3, 2025 22:28

test(teams): report diagnostics for test failures

d70134e

test(rma): report diagnostics for test failures

e892ce2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test with Julienne #169

Test with Julienne #169

rouson commented Dec 28, 2024 •

edited

Loading

everythingfunctional left a comment

rouson commented Jan 2, 2025

bonachea left a comment

bonachea Jan 2, 2025

bonachea Jan 2, 2025

bonachea Jan 2, 2025

bonachea Jan 2, 2025

	call_assert(len(lhs) == len(rhs))
	call assert_always(len(lhs) == len(rhs), "length parameter mismatch in alphabetize")

	call_assert(init_status==successful)
	call assert_always(init_status==successful, "prif_init failed")

Test with Julienne #169

Are you sure you want to change the base?

Test with Julienne #169

Conversation

rouson commented Dec 28, 2024 • edited Loading

To Do

everythingfunctional left a comment

Choose a reason for hiding this comment

rouson commented Jan 2, 2025

bonachea left a comment

Choose a reason for hiding this comment

bonachea Jan 2, 2025

Choose a reason for hiding this comment

bonachea Jan 2, 2025

Choose a reason for hiding this comment

bonachea Jan 2, 2025

Choose a reason for hiding this comment

bonachea Jan 2, 2025

Choose a reason for hiding this comment

rouson commented Dec 28, 2024 •

edited

Loading